London | 26-ITP-May | Zadri Abdule | Sprint 2 | Book - library - #520
London | 26-ITP-May | Zadri Abdule | Sprint 2 | Book - library#520Zadri415 wants to merge 3 commits into
Conversation
Zadri415
added
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Flows/blob/general-review-feedback/debugging/book-library/feedback.md
Doing so can help me speed up the review process. Thanks.
cjyuan
added
Reviewed
Zadri415
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good.
There are a few improvements you could still make.
| #toast { | ||
| position: fixed; |
There was a problem hiding this comment.
Why not prepare the CSS in style.css?
| /> | ||
| <div class="col"> | ||
| <label for="pages">Pages:</label> | ||
| <input type="number" class="form-control" id="pages" name="pages" min="1" required /> |
There was a problem hiding this comment.
Could you find out why the browser is not enforcing the constraint min="1" required? Currently a user could submit page count suchas-123and12.345`.
| @@ -1,103 +1,137 @@ | |||
| let myLibrary = []; | |||
There was a problem hiding this comment.
Can we declare myLibrary in a way that prevents it from being accidentally reassigned?
| // Preprocessing / validation: | ||
| // - reject empty or whitespace-only title/author (checked AFTER trim, not before) | ||
| // - reject non-numeric or non-positive page counts | ||
| if (!title || !author || !pagesRaw || !Number.isFinite(pages) || pages <= 0) { |
There was a problem hiding this comment.
-
Do you know why checking
!pageRawis optional? -
What type of number should a page count be?
| titleInput.value = ""; | ||
| authorInput.value = ""; | ||
| pagesInput.value = ""; | ||
| checkInput.checked = false; |
There was a problem hiding this comment.
If the input elements are inside a <form>, we could also just reset the form with one function call.
| const table = document.getElementById("display"); | ||
| const tbody = table.getElementsByTagName("tbody")[0]; |
There was a problem hiding this comment.
Note: We could also use document.querySelector() to select the specific tbody element directly.
| const idx = myLibrary.indexOf(book); | ||
| if (idx === -1) return; |
There was a problem hiding this comment.
We could also obtain the index of the current book from the second parameter of the function pass to forEach() on line 66.
cjyuan
removed
the
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
Questions
N/A